What does that mean "otherwise there is no title generated"?

That second argument is useful. It's always external. More importantly, isExternal shouldn't be set in getSheetFromTextFn at all. It belongs in the same function that you set isExternal = false (this one).

Solved!

You should add && sheet.cssRules.length <= 0, rather than add an if statement in the try/catch block. Currently the code looks like it's missing an "else".

We cannot move .cssRules to here, as we need the catch to trigger for external stylesheets. Trying to read a .cssRules on external resource throws a SecurityError, which flows into the catch block. This is documented below.

Although I have refactored slightly

If we're not doing shadow DOM in this PR, than we need to replace this expression. Otherwise we're just committing buggy shadow DOM code to develop, rather than not having it supported.

I have just solved for shadowDOM, and it would make sense to fold all of that into the same PR. So yes, shadowDOM is in the same PR.

preload=false shouldn't return false in isValidPreloadObject. This is misleading.

One more thing we forgot to test is if cross domain sheets loaded through @import can be accessed. I suspect they might not, so there's still some work in there I think.

Indeed, you are right. I have come up with a solution for the same. Which is why shadowDOM work is going into this PR as against a new one.

Note:
Basic one-level deep nested @import is tackled here.
Have created an issue to tackle further enhancements - #1054

This code will never run because if it isn't, your shouldPreload function causes utils.preload to exit early.

You could have done return out.concat(cssomSheets).

I'd suggest we run this first and throw if its invalid, rather than do it in getPreloadConfig.

That is what we are doing.

const shouldPreload = axe.utils.shouldPreload(options);
if (!shouldPreload) {
	return q;
}

const preloadConfig = axe.utils.getPreloadConfig(options);

It isn't. I put this in the page-has-heading-one tests to make sure I can test that page without mocha getting in the way. That's the only place its needed.

Do you mean, "should reject if external sheet fails to load"?

I'm having a very hard time understanding what's happening here. As best I can tell you're trying to turn same origin sheets and style elements with import statements into one long stylesheet... that doesn't seem like a good idea to me. This means we lose all context of how why and where a stylesheet got included. It also ignores that @import can have its own media value, and that they can be recursive. We also shouldn't ignore @import on cross-origin sheets.

Please be sure to include tests that covers all of these things.

Note: Paired on the above, refactored as discussed.

This could be a 1-liner: const getCssomSheet = (sheet, isExternal, shadowId) => ({ sheet, isExternal, shadowId }).

Also, this function seems unnecessary.

Can remove the .hasOwnProperty check, as Array.isArray(undefined) returns false.

Could simplify this:

axe.utils.shouldPreload = options => options && isValidPreloadObject(options.preload)

Why was my comment marked as resolved? You didn't change anything or respond.....?

Simply because, I do not want to check if preload is a valid object until I am sure that it is an object. Hence a boolean check and an undefined check earlier.

Preload can take 3 values: true, false or { assets: ['assetKey'] } as defined in the docs.

Doing this options && isValidPreloadObject(options.preload) ignores and overrides the boolean values due to &&.

Scenarios (with your suggestion):

I am sure there is a clever way to shorten things further, but this keeps it easy to follow.
Prefer to keep this for that reason.

Appreciate the comments.

Discussed on Slack. Disregard my comment. This is good as-is!

IIRC we're not capitalizing the "x" anymore.

to avoid i18n concerns removed aXe.

This seems like something that needs to be fixed first. Duplicating these functions will lead to maintenance nightmares.

Should we add a warning here when this method is called? Just aliasing it doesn't seem to give users any reason not to use it.

Something like:

let didWarn = false
dom.getRootNode = (...args) => {
  if (!didWarn) {
    console.warn('axe-core: dom.getRootNode has been deprecated; use utils.getRootNode instead.')
    didWarn = true
  }
  axe.utils.getRootNode(...args)
}

I like this.

Reason I did not do this is right away is, we are bypassing the function to axe.utils for the time being.

The jsdoc @deprecated notice is a flag for the dev's (us) to take some action on a later date.

There are multiple places with in the code base still referring to axe.commons.dom.getRootNode, until we change all these references internally first, to axe.utils.getRootNode believe we should not warn on the console.

And before all the internal changes come be made, this PR needs to be merged.

Also reckon a deprecation notice should dictate when an API is going to be deprecated, so we need to agree on that, something like this function has been deprecated from v3.2.0, use newFunction.

Happy to add this, once we reach that consensus.

@marcysutton @WilcoFiers - thoughts?

Thanks @stephenmathieson

This .reduce() seems to be performing the function of .map(). Should we refactor to use the intended native method here?

I am taking each CSSRule and appending its cssText in to an array - out. I intend to keep this as reduce, as is, because I believe there will be logic going into this iteration as I tackle for @import and media rules.

Appreciate the comment.